feat(stats): split off transmission to RawSender, implement batching and queueing support, add streamlined prefix and suffix support
#6267
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
kwvg
force-pushed
the
statsd2
branch
2 times, most recently
from
1b1c768
to
1b87c07
Compare
PastaPastaPasta
added a commit
that referenced
this pull request
Sep 11, 2024
4faf35f chore: add `stats` as a pull request header scope (Kittywhiskers Van Gogh) Pull request description: ## Additional Information Would allow tagging #5167, #6267 and #6237 as `feat(stats)`. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code **(note: N/A)** - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ **(note: N/A)** ACKs for top commit: PastaPastaPasta: utACK 4faf35f UdjinM6: utACK 4faf35f thephez: utACK 4faf35f Tree-SHA512: 25cc28792351852b9e920d980b8814d93274bc53d22ce63fb1a5bf32821b10902d22b384a408e1c4a7b97239e53e235c45ac008d0f82e1afe5d6071b392acb47
|
This pull request has conflicts, please rebase. |
|
Should update diff --git a/test/util/data/non-backported.txt b/test/util/data/non-backported.txt
index 150bbb70c1..7684074874 100644
--- a/test/util/data/non-backported.txt
+++ b/test/util/data/non-backported.txt
@@ -29,7 +29,8 @@ src/rpc/quorums.cpp
src/saltedhasher.*
src/spork.*
src/stacktraces.*
-src/statsd_client.*
+src/stats/*.cpp
+src/stats/*.h
src/test/block_reward_reallocation_tests.cpp
src/test/bls_tests.cpp
src/test/dip0020opcodes_tests.cpp |
|
not sure if can actually do d42c88f develop: this PR: |
old ideas
diff --git a/src/stats/client.cpp b/src/stats/client.cpp
index cdcf36d281..b536a44db3 100644
--- a/src/stats/client.cpp
+++ b/src/stats/client.cpp
@@ -157,6 +157,8 @@ bool StatsdClient::send(const std::string& key, T1 value, const std::string& typ
return true;
}
+ if (value < EPSILON) value = 0;
+
// Construct the message and if our message isn't always-send, report the sample rate
std::string buf{strprintf("%s%s%s:%s|%s", m_prefix, key, m_suffix, ToString(value), type)};
if (frequency < 1.f) {
diff --git a/src/stats/client.cpp b/src/stats/client.cpp
index cdcf36d281..832085c8ef 100644
--- a/src/stats/client.cpp
+++ b/src/stats/client.cpp
@@ -6,7 +6,6 @@
#include <stats/client.h>
-#include <util/string.h>
#include <util/system.h>
#include <cmath>
@@ -25,6 +24,7 @@ static constexpr char STATSD_NS_DELIMITER{'.'};
static constexpr char STATSD_METRIC_COUNT[]{"c"};
/** Character used to denote Statsd message type as gauge */
static constexpr char STATSD_METRIC_GAUGE[]{"g"};
+static constexpr char STATSD_METRIC_GAUGE_DOUBLE[]{"g"};
/** Characters used to denote Statsd message type as timing */
static constexpr char STATSD_METRIC_TIMING[]{"ms"};
} // anonymous namespace
@@ -133,7 +133,7 @@ bool StatsdClient::gauge(const std::string& key, int64_t value, float frequency)
bool StatsdClient::gaugeDouble(const std::string& key, double value, float frequency)
{
- return send(key, value, STATSD_METRIC_GAUGE, frequency);
+ return send(key, value, STATSD_METRIC_GAUGE_DOUBLE, frequency);
}
bool StatsdClient::timing(const std::string& key, uint64_t ms, float frequency)
@@ -158,7 +158,12 @@ bool StatsdClient::send(const std::string& key, T1 value, const std::string& typ
}
// Construct the message and if our message isn't always-send, report the sample rate
- std::string buf{strprintf("%s%s%s:%s|%s", m_prefix, key, m_suffix, ToString(value), type)};
+ std::string buf;
+ if (type == STATSD_METRIC_GAUGE_DOUBLE) {
+ buf = strprintf("%s%s%s:%f|%s", m_prefix, key, m_suffix, value, type);
+ } else {
+ buf = strprintf("%s%s%s:%d|%s", m_prefix, key, m_suffix, value, type);
+ }
if (frequency < 1.f) {
buf += strprintf("|@%.2f", frequency);
}actually, diff --git a/src/stats/client.cpp b/src/stats/client.cpp
index cdcf36d281..55f5f458c0 100644
--- a/src/stats/client.cpp
+++ b/src/stats/client.cpp
@@ -6,7 +6,6 @@
#include <stats/client.h>
-#include <util/string.h>
#include <util/system.h>
#include <cmath>
@@ -158,7 +157,7 @@ bool StatsdClient::send(const std::string& key, T1 value, const std::string& typ
}
// Construct the message and if our message isn't always-send, report the sample rate
- std::string buf{strprintf("%s%s%s:%s|%s", m_prefix, key, m_suffix, ToString(value), type)};
+ std::string buf{strprintf("%s%s%s:%.6f|%s", m_prefix, key, m_suffix, value, type)};
if (frequency < 1.f) {
buf += strprintf("|@%.2f", frequency);
}
|
UdjinM6
reviewed
Sep 12, 2024
UdjinM6
reviewed
Sep 12, 2024
|
This pull request has conflicts, please rebase. |
In upcoming commits, message sending will be split off into a separate file and stats capabilities will be fleshed out. Prepare for that by giving it its own directory. Also get rid of `statsd` namespace, it is entirely unnecessary. Co-authored-by: UdjinM6 <[email protected]>
kwvg
changed the title
RawSender, implement batching and queueing support, add streamlined suffix and prefix supportRawSender, implement batching and queueing support, add streamlined prefix and suffix support
UdjinM6
reviewed
Sep 12, 2024
`RawSender` is inspired by `UDPSender` from `vthiery/cpp-statsd-client` and separating it out of `StatsdClient` is needed to implement queueing and batching support in upcoming commits. This is the start of migrating our Statsd codebase to `cpp-statsd-client`.
Co-authored-by: UdjinM6 <[email protected]>
UdjinM6
approved these changes
Sep 13, 2024
Additionally, let's remove the key sanitation logic since keys are hardcoded. Co-authored-by: UdjinM6 <[email protected]>
- `timing` cannot contain a negative value, change to `uint64_t` - remove `sendDouble`, use template specialization of `Send` instead, that can be reused as `send` - rename `value` in `count()` to `delta` - add more comments `gaugeDouble` has been kept around because utilizing templates for it would require us to either write template specializations for every kind of `gauge` value (implicit conversions will not save us from this) or move all logic to the header, neither option seems desirable.
This is to avoid the odd circumstance where stats don't work because you specified everything but didn't explicitly enable it. Using `-statsenabled` has been deprecated and will be removed in the next major release.
`statshostname` implies that alongside specifying the host, the user needs to specify a separate "hostname" field in order to connect when in fact, it is an entirely optional suffix field applied to stats keys. Rename it to `statssuffix` and deprecate `statshostname`, the latter will be removed in a subsequent major release.
`statsns`, aside from being an unclear name, in its default behaviour, doesn't use the namespace delimiter contaminates the root namespace of the key. Let's take care of that by deprecating `statsns` with its replacement, `statsprefix`, that behaves properly.
This marks the completion of our transition from code based on `talebook/statsd-client-cpp` to code based on `vthiery/cpp-statsd-client`. Also, we long stopped using `snprintf`, remove it from exclusions list.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This pull request achieves the goal originally set out in dash#5167, to migrate the base of our Statsd client implementation to one that is actively maintained. Statoshi (source) utilizes talebook/statsd-client-cpp, which in turn is inherited by Dash.
As Statsd is the only cross-platform reporting mechanism available (USDT requires a Linux host with superuser privileges and RPCs don't provide as much flexibility as desired), emphasis is placed on using Statsd as the primary way for the Dash daemon to report metrics related to node and network health.
As part of maintaining our Statsd client, this PR aims to migrate the base of our implementation to vthiery/cpp-statsd-client, a streamlined implementation that embraces C++11 with a thread-safe implementation of queueing and batching, which reduces the number of packets used to transmit stats.
These capabilities are optional and users can opt not to use them by setting
-statsduration=0to disable queueing (which will also disable batching) or-statsbatchsize=0, which will disable batching (but will not disable queueing unless requested explicitly).Additional Information
statsd::StatsdClient, make globalunique_ptr#5167RawSender(and by extension,RawMessage) strive to remain as unopinionated as possible, moving the responsibility to construct valid Statsd messages ontoStatsdClient. This is to ensure thatRawSendercan be reused down the line or independently extended without impacting the Statsd client.RawMessageexists to provide extensions tostd::vector<uint8_t>that make it easier to abstract away strings while also implementing some of its semantics likeappend()(and its alias,+=).InitStatsClient()was introduced to keepStatsdClientindifferent to how arguments are obtained and sanitized before they're supplied to the constructor. This is to keep it indifferent to all the backwards-compatibility code for deprecated arguments still work.%fwithout having to specify a precision as tinyformat automatically assumes a precision of 6 (source) and problems don't seem to be observed when using%fwith integers (source).static_assertto ensure that a specialization ofsend()involving a non-arithmetic type will raise alarm when compiling (source).Breaking changes
-statsenabled(replaced with specifying-statshost),-statshostname(replaced by-statssuffix) and-statsns(replaced by-statsprefix) have been deprecated and will be removed in a future release.Checklist: